Closed
Bug 813041
Opened 12 years ago
Closed 11 years ago
always layerize the scroll indicator
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
People
(Reporter: gal, Assigned: mattwoodrow)
References
Details
Attachments
(4 files, 4 obsolete files)
14.02 KB,
patch
|
Details | Diff | Splinter Review | |
12.77 KB,
patch
|
Details | Diff | Splinter Review | |
1.66 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.38 KB,
patch
|
Details | Diff | Splinter Review |
Right now we repaint the scroll indicator every time we move it. Its just a simple nsDisplayBackground so thats not too bad. However, we also have to repaint the area that becomes visible underneith it, which sucks and can be slow. We should put the indicator into its own layer. Its smallish anyway.
Reporter | ||
Comment 1•12 years ago
|
||
What do you guys think?
Reporter | ||
Comment 2•12 years ago
|
||
// We put scrollbars in their own layers when this is the root scroll // frame and we are a toplevel content document. In this situation, the // scrollbar(s) would normally be assigned their own layer anyway, since // they're not scrolled with the rest of the document. But when both // scrollbars are visible, the layer's visible rectangle would be the size // of the viewport, so most layer implementations would create a layer buffer // that's much larger than necessary. Creating independent layers for each // scrollbar works around the problem. bool createLayersForScrollbars = mIsRoot && mOuter->PresContext()->IsRootContentDocument(); I don't think this is kicking in, otherwise we shouldn't see invalidation.
Reporter | ||
Comment 3•12 years ago
|
||
Actually, its kicking in, and we invalidate anyway, which is a bug. So this is invalid.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 4•12 years ago
|
||
Actually, not invalid after all. We layerize, but we still invalidate. Instead, we want to detect that we aren't actually changing what we paint, just moving the scrollbox thumb.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → matt.woodrow
Assignee | ||
Comment 5•12 years ago
|
||
Apparently drawing the scrollbar costs b2g around 10fps when scrolling, so maybe this is worthwhile. This completely breaks scrollbars on mac because the SliderFrame/thumb frame that we create don't actually paint anything and the entire scrollbar (thumb and all) is painted by ScrollbarFrame. The 'thumb' display items that are created do nothing except trick DLBI into invalidating the correct pixels. Shifting these items into their own layer stops this, so we never actually repaint the scrollbar. I think this should work on b2g though, assuming we do sane things for painting the scrollbar there. Does this look like a reasonable approach roc? Obviously needs a fair bit of work, the perspective change to force an active layer is my favourite bit :)
Attachment #683459 -
Flags: feedback?(roc)
Comment 6•12 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #5) > This completely breaks scrollbars on mac because the SliderFrame/thumb frame > that we create don't actually paint anything and the entire scrollbar (thumb > and all) is painted by ScrollbarFrame. So nsNativeThemeCocoa::WidgetStateChanged should probably return true if aWidgetType is NS_THEME_SCROLLBAR or NS_THEME_SCROLLBAR_SMALL and aAttribute is curpos, minpos, maxpos or pageincrement. That would also have been the right fix to bug 385058 (which was my first patch ever).
Why are we putting the whole slider into a layer instead of just the thumb?
Assignee | ||
Comment 8•12 years ago
|
||
We don't? The nsDisplayTransform is only wrapping the children of the nsSliderFrame.
Assignee | ||
Comment 9•12 years ago
|
||
Thanks Markus. Looks like I inadvertently reverted bug 385058 with DLBI. I haven't seen any regressions filed, so I guess we no longer use a theme that paints outside the bounds of the thumb frame. Your suggestion works, but now we're back to painting the entire scrollbar instead of only the thumb area. Maybe that's not an issue, since it's what we were doing pre-DLBI too. Should only affect OSX. It would be nice if we could paint the thumb separately to the scrollbar, or have WidgetStateChanged return a rect to invalidate. Probably outside the scope of this bug though.
Comment on attachment 683459 [details] [diff] [review] WIP: Shift the scrollbar thumb using an active transform Review of attachment 683459 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/xul/base/src/nsSliderFrame.h @@ +129,5 @@ > + temp = gfx3DMatrix::Translation(NSAppUnitsToFloatPixels(mSliderOffset, aAppUnitsPerPixel), 0, 0); > + } else { > + temp = gfx3DMatrix::Translation(0, NSAppUnitsToFloatPixels(mSliderOffset, aAppUnitsPerPixel), 0); > + } > + temp._43 = 1; Why is this line needed?
Attachment #683459 -
Flags: feedback?(roc) → feedback+
Reporter | ||
Comment 11•12 years ago
|
||
With the attached patch we are still invalidating the scroll bar thumb every time it moves.
Assignee | ||
Comment 12•12 years ago
|
||
Sorry, forgot to upload the latest patch. This one works (I hope). Still failing some tests on try though currently.
Attachment #683459 -
Attachment is obsolete: true
Blocks: 876741
Comment 13•11 years ago
|
||
I just rebased the WIP to play with it since I would like to turn the scrollbars on again for b2g (bug 876741). Works well for me.
Comment 14•11 years ago
|
||
Same rebased wip, but this one compiles...
Attachment #819742 -
Attachment is obsolete: true
Blocks: 876741
Comment 15•11 years ago
|
||
So... it does block bug 876741? Since that one is koi+, this one should be as well. Can somebody clarify the dependency?
blocking-b2g: --- → koi?
Comment 16•11 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #15) > So... it does block bug 876741? Since that one is koi+, this one should be > as well. Can somebody clarify the dependency? (In reply to Milan Sreckovic [:milan] from comment #15) > So... it does block bug 876741? Since that one is koi+, this one should be > as well. Can somebody clarify the dependency? Please see comment 5. "Apparently drawing the scrollbar costs b2g around 10fps when scrolling, so maybe this is worthwhile."
Comment 18•11 years ago
|
||
NI :mattwoodrow to give an eta on fixing this and set a 1.2 target milestone as this blocks 876741
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 19•11 years ago
|
||
I haven't looked at this since 2012, I assumed Vivien was working on this and just hadn't changed the assignee. FWIW, I don't think the existing prototype is a great way to go about this, doing something similar to bug 876321 would be easier.
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 20•11 years ago
|
||
Can you let me know if this works for b2g? I know of at least two issues with this patch: 1) It still triggers invalidation for desktop since the scrollbar thumb moves by subpixel amounts. We don't retain content for subpixel moves since it tends to look bad, but we ignore this for mobile (for performance reasons). 2) It will break scrollbars for the old (10.6?) style OSX scrollbars, though maybe only if 1 is fixed. These scrollbars render the entire thing (track, thumb etc) as a single display item, and the thumb display item is just a placeholder to trigger invalidation. If we're not invalidating it, then nothing will trigger the real item behind it to be repainted.
Attachment #827005 -
Flags: feedback?(21)
Comment 21•11 years ago
|
||
Comment on attachment 827005 [details] [diff] [review] Push the scrollbar into it's own ThebesLayer I'm not working on this patch fwiw, sorry for the miscommunication issue. About panning it seems to not regress too with this patch.
Attachment #827005 -
Flags: feedback?(21) → feedback+
Assignee | ||
Comment 22•11 years ago
|
||
Comment on attachment 827005 [details] [diff] [review] Push the scrollbar into it's own ThebesLayer Roc, how would you feel about putting this inside a mobile/b2g only #ifdef and landing it as is? Getting it to work properly on desktop is fairly tricky, and would require runtime checks to check what type of scrollbars are enabled. I don't think we want that overhead in GetActiveScrolledRootFor. Besides, it's unlikely to ever matter performance wise for desktop.
Attachment #827005 -
Flags: review?(roc)
Comment on attachment 827005 [details] [diff] [review] Push the scrollbar into it's own ThebesLayer Review of attachment 827005 [details] [diff] [review]: ----------------------------------------------------------------- Replace the do_QueryFrame check below with a type check using the cached result of GetType(), so we don't add any virtual calls here.
Attachment #827005 -
Flags: review?(roc) → review-
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #827005 -
Attachment is obsolete: true
Attachment #828510 -
Flags: review?(roc)
Comment on attachment 828510 [details] [diff] [review] Push the scrollbar into it's own ThebesLayer v2 Review of attachment 828510 [details] [diff] [review]: ----------------------------------------------------------------- Patch is unchanged?
Attachment #828510 -
Flags: review?(roc) → review-
Assignee | ||
Comment 26•11 years ago
|
||
Now with more qref.
Attachment #828510 -
Attachment is obsolete: true
Attachment #828520 -
Flags: review?(roc)
Attachment #828520 -
Flags: review?(roc) → review+
Assignee | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c40724f1a0e
https://hg.mozilla.org/mozilla-central/rev/6c40724f1a0e
Status: REOPENED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment on attachment 828520 [details] [diff] [review] Push the scrollbar into it's own ThebesLayer v3 Review of attachment 828520 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +1265,5 @@ > + // Treat the slider thumb as being as an active scrolled root > + // on mobile so that it can move without repainting. > + if (parentType == nsGkAtoms::sliderFrame) > + break; > +#endif Hmm ... actually this needs to apply to b2g too. I suggest we add a method to the slider frame to check whether it should be treated as an active scrolled root, and that method should call LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars).
Comment 30•11 years ago
|
||
Holding off on the b2g26 uplift pending comment 29.
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > Comment on attachment 828520 [details] [diff] [review] > Push the scrollbar into it's own ThebesLayer v3 > > Review of attachment 828520 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsLayoutUtils.cpp > @@ +1265,5 @@ > > + // Treat the slider thumb as being as an active scrolled root > > + // on mobile so that it can move without repainting. > > + if (parentType == nsGkAtoms::sliderFrame) > > + break; > > +#endif > > Hmm ... actually this needs to apply to b2g too. > > I suggest we add a method to the slider frame to check whether it should be > treated as an active scrolled root, and that method should call > LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars). ANDROID should be defined on b2g too I believe. As for getting it to work on desktop, I think we just need to follow Markus' suggestion from comment 6. And the change the nsSliderFrame code to snap the thumb positions to integer pixel positions. The latter part seemed likely to break reftests though, and I doubt it's much of a performance win for desktop. Assuming that I'm right about the #define, then I think we should just stick with that and uplift it. We can file a follow-up for desktop if you think it's worthwhile. Thoughts?
Flags: needinfo?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #31) > Assuming that I'm right about the #define, then I think we should just stick > with that and uplift it. > > We can file a follow-up for desktop if you think it's worthwhile. Thoughts? OK.
Flags: needinfo?(roc)
Comment 33•11 years ago
|
||
Not entirely sure how to resolve this for b2g26. Please post a branch-specific patch :)
status-b2g-v1.2:
--- → affected
status-firefox26:
--- → wontfix
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
Flags: needinfo?(matt.woodrow)
Keywords: branch-patch-needed
Assignee | ||
Comment 34•11 years ago
|
||
Flags: needinfo?(matt.woodrow)
Comment 36•11 years ago
|
||
Sorry about that, there was a flaw in my bug queries that caused me to miss this. I've fixed it so it shouldn't happen again. https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c60fd54b599b
You need to log in
before you can comment on or make changes to this bug.
Description
•